fix: parse adjacency-list values for non-root paths #712
Conversation
In `DataModel.update`, `_parseDataModelContents` was only called when the path was the root. For non-root paths, this caused a TypeError when a widget subscribed to that path expecting a map since the stored value was still the raw A2UI adjacency list format.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the issue of parsing adjacency-list values for non-root paths by introducing a parsedContents variable. This ensures that list-based content is correctly transformed into a map structure before being applied to the data model, whether at the root or a nested path. The addition of a dedicated test case for non-root path parsing further validates the fix and improves test coverage. The changes are clear, concise, and directly resolve the reported problem.
andrewkolos
force-pushed
the
data-model-fix
branch
from
784cbce to
2bbfb50
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where adjacency-list values were not being parsed for non-root paths, involving detecting and parsing adjacency lists before updating the data model, and includes a new test case. However, a medium-severity security issue was identified concerning the logging of untrusted data in the update method, which could expose sensitive information and cause denial-of-service. Additionally, the review includes suggestions to address unreachable code, a potential type-safety issue in the root path update logic, and to refactor a loop into a more idiomatic functional style.
| genUiLogger.warning( | ||
| 'DataModel.update: literal list cannot be used as ' | ||
| 'root data model: $contents', | ||
| ); |
There was a problem hiding this comment.
The DataModel.update method logs the entire contents object at the WARNING level when a literal list is used as the root data model. This is a medium-severity security issue as it can lead to sensitive information exposure (PII, tokens, etc.) in the application logs. Additionally, string interpolation of large or deeply nested objects can cause performance issues or even a stack overflow due to recursive toString() calls in Dart's collection classes. It is recommended to remove the object from the log message to prevent data leakage and ensure stability.
| genUiLogger.warning( | |
| 'DataModel.update: literal list cannot be used as ' | |
| 'root data model: $contents', | |
| ); | |
| genUiLogger.warning( | |
| 'DataModel.update: literal list cannot be used as ' | |
| 'root data model', | |
| ); |
| bool _isAdjacencyList(List<Object?> contents) { | ||
| if (contents.isEmpty) return false; | ||
| for (final item in contents) { | ||
| if (item is! Map) return false; | ||
| if (!item.containsKey('key')) return false; | ||
|
|
||
| if (!_a2uiValueKeys.any(item.containsKey)) return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
For improved conciseness and to follow a more functional style, you could consider using the every method on the list. This checks if all items satisfy the conditions for being part of an adjacency list and can make the intent of the code slightly clearer.
bool _isAdjacencyList(List<Object?> contents) {
if (contents.isEmpty) return false;
return contents.every((item) {
if (item is! Map || !item.containsKey('key')) {
return false;
}
return _a2uiValueKeys.any(item.containsKey);
});
}1 participant
Description
Fixes #710. That issue has a detailed technical breakdown, so I won't duplicate one here.
Pre-launch Checklist
///).